Fix #140, #167, address a comment in #144 and rework the bearer middleware tests #168

kevinchalet · 2015-03-03T15:03:43Z

The PR is a bit noisy but I had to rework large parts of various tests, that were not really conclusive.

(GitHub accidentally closed my previous PR when I rebased it (#142) so I had to re-submit it)

ghost · 2015-03-03T15:03:47Z

Hi @PinpointTownes, I'm your friendly neighborhood Microsoft Open Technologies, Inc. Pull Request Bot (You can call me MSOTBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft Open Technologies, Inc. and real humans are currently evaluating your PR.

TTYL, MSOTBOT;

kevinchalet · 2015-03-03T15:20:46Z

I also included fixes for #144 (comment), #167 and the casing issue of OpenidConnectAuthenticationHandler.cs

kevinchalet · 2015-03-03T15:21:58Z

/cc @brentschmaltz @Praburaj @Tratcher

Praburaj · 2015-03-05T04:36:25Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs

@@ -566,7 +565,7 @@ public override Task<bool> InvokeAsync()
            {
                if (ticket.Principal != null)
                {
-                    Request.HttpContext.Response.SignIn(ticket.AuthenticationScheme, ticket.Principal, ticket.Properties);
+                    Request.HttpContext.Response.SignIn(Options.SignInScheme, ticket.Principal, ticket.Properties);


I think this change is already in.

/CC @HaoK

Only this one line is in, so yeah a rebase probably would make this diff go away, this PR has more than that fix though, I am not familiar enough with this code to determine whether these other changes are appropriate

@Praburaj rebased, thanks 👯

Praburaj · 2015-03-05T18:49:12Z

Changes look good to me. @tushargupta51 - could you also review?

brentschmaltz · 2015-03-05T21:39:40Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationMiddleware.cs

@@ -46,9 +45,14 @@ public class OpenIdConnectAuthenticationMiddleware : AuthenticationMiddleware<Op
        {
            _logger = loggerFactory.CreateLogger<OpenIdConnectAuthenticationMiddleware>();

+            if (string.IsNullOrEmpty(Options.SignInScheme))


What is SignInScheme used for?

tushargupta51 · 2015-03-05T22:43:05Z

The changes to the bearer middleware tests looks good.

kevinchalet · 2015-03-05T22:58:31Z

src/Microsoft.AspNet.Authentication.Cookies/CookieAuthenticationMiddleware.cs

@@ -4,8 +4,6 @@
 using System;
 using Microsoft.AspNet.Authentication.Cookies.Infrastructure;
 using Microsoft.AspNet.Authentication.DataHandler;
-using Microsoft.AspNet.Authentication.Cookies.Infrastructure;


Totally unrelated changes, but these 2 lines were causing compilation warnings.

Fixed a few minutes ago by @HaoK: e2bb762

kevinchalet · 2015-03-10T19:03:47Z

Rebased.

kevinchalet · 2015-03-15T13:36:33Z

Squashed to incorporate feedback in the appropriate commits.

kevinchalet · 2015-03-18T10:01:24Z

Conflict fixed.

@HaoK @tushargupta51 @brentschmaltz any chance you could have a final look and merge it? 😄

HaoK · 2015-03-19T07:45:42Z

I'll try to get to this after my hosting changes are done...

kevinchalet · 2015-03-19T08:03:11Z

@HaoK thanks. Don't hesitate to ping me if there are conflicts that need to be fixed.

kevinchalet · 2015-04-09T17:52:14Z

I took the liberty to fix OpenIdConnectAuthenticationOptions.SignInScheme.

@HaoK should we also update OAuthAuthenticationOptions, TwitterAuthenticationOptions and ExternalAuthenticationOptions to align the documentation?

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuth/OAuthAuthenticationOptions.cs#L104
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Twitter/TwitterAuthenticationOptions.cs#L94
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication/ExternalAuthenticationOptions.cs#L11

HaoK · 2015-04-09T18:33:58Z

Yeah looks good, go ahead and normalize the doc comments to be consistent, I'll merge this in soon

HaoK · 2015-04-09T18:37:27Z

I'm not suggesting you make this change in this PR, but what do you think about maybe ExternalAuthenticationOptions -> SignInSchemeOptions is that better or worse? I'm not sure what's a good property name if we did that, Scheme or AuthenticationScheme perhaps?

kevinchalet · 2015-04-09T18:50:21Z

Personally, I think I'd simply remove ExternalAuthenticationOptions and rely exclusively on OAuthAuthenticationOptions/OpenIdConnectAuthenticationOptions/TwitterAuthenticationOptions to set up SignInScheme: I'm convinced it would make this "delegated persistence" stuff more evident and easier to understand for end developers.

We could also set a default value (Cookies to match the default scheme for the cookies middleware? But IIRC, that's not the default scheme used by aspnet/Identity) for these properties, so people wouldn't have to set an explicit value for basic cookies + social provider scenarios..

HaoK · 2015-04-09T19:01:52Z

Yeah so the idea is we want to easily configure all of the auth middlewares via one central services.Configure(options => options.SignInScheme = "Cookie"). Otherwise any app that's using Google/Facebook/Twitter/MAuth would need to do this for every auth middleware mechanically (and the error story if you misconfigure this is pretty poor)

HaoK · 2015-04-09T19:04:54Z

The intent is basically to be the centralized default SignInScheme, which might not be a bad name: SignInOptions.DefaultSignInScheme, as all auth middlewares would use this as the default scheme for their SignInScheme

kevinchalet · 2015-04-09T19:14:48Z

Indeed, removing ExternalAuthenticationOptions would make global SignInScheme more painful to set up as you'd have to configure it for each authentication middleware 😄

Another approach: the OIDC/OAuth2/Twitter options could inherit from ExternalAuthenticationOptions and stop exposing a custom SignInScheme. The default scheme could be configured via services.Configure<ExternalAuthenticationOptions>(options => options.SignInScheme = "Cookie") as you suggested, and it could be replaced by services.Configure<OpenIdConnectAuthenticationOptions>(options => options.SignInScheme = "Other").

That said, I'm not sure it would work as-is with the current aspnet/Options bit. In this case, would OpenIdConnectAuthenticationOptions automagically be created with the settings applied to ExternalAuthenticationOptions it inherits from?

SignInOptions.DefaultSignInScheme sounds like a good name, though it's not obvious it would only be supported by OIDC/OAuth2/Twitter providers.

HaoK · 2015-04-09T19:24:29Z

Yeah configuration options doesn't really look at inheritance, you are configuring a singleton per TOptions, so you'd be configuring only the base options, which if noone is asking for, will be ignored. So that doesn't work unfortunately.

Yeah perhaps we still need something that sounds like External/Delegating/NonAutomatic something that conveys that this is for the non automatic auth middlewares to use.

[GoodNameForNonAutomatic]AuthenticationOptions.DefaultSignInScheme i think is the correct name, just need to find a replacement for External

HaoK · 2015-04-09T19:26:38Z

Perhaps we could just use a grab all SharedAuthenticationOptions.DefaultSignInScheme?

@Tratcher what do you think about renaming ExternalAuthenticationOptions to be something more general?

Tratcher · 2015-04-09T19:45:02Z

@HaoK Sounds fine. It doesn't have any other fields.

brentschmaltz · 2015-04-09T21:32:06Z

@HaoK Help me understand what SignInScheme is used for and if it is completely implemented?

HaoK · 2015-04-09T23:21:28Z

@brentschmaltz refer to @PinpointTownes update to the doc comments, its fairly clear, nothing has changed in its meaning or intent since Katana, we just renamed it from SignInAsAuthenticationType

https://github.com/PinpointTownes/Security/commit/39a970c8f25b685e462f165aa768c0c929cbbad3

kevinchalet · 2015-04-10T14:08:41Z

@brentschmaltz if you think it's not clear enough, creating a wiki page with various code samples and copying the URL in the documentation might be a good option.

HaoK · 2015-04-13T21:02:57Z

@PinpointTownes Do you want to squash your commits down to one or two? If I do it, the changes will look like mine instead of yours.

…dleware tests

kevinchalet · 2015-04-13T21:21:38Z

@HaoK done. I hope the commits will be descriptive enough 😄

ghost added the cla-not-required label Mar 3, 2015

kevinchalet changed the title ~~Fix #140 and rework the different OAuth2 Bearer middleware tests~~ Fix #140, #167, address a comment in #144 and rework the bearer middleware tests Mar 3, 2015

kevinchalet mentioned this pull request Mar 3, 2015

Move Active Authentication Mode into AutomaticAuthenticationHandler #164

Closed

Praburaj reviewed Mar 5, 2015
View reviewed changes

brentschmaltz reviewed Mar 5, 2015
View reviewed changes

kevinchalet reviewed Mar 5, 2015
View reviewed changes

This was referenced Apr 2, 2015

OAuthBearerAuthenticationHandler does not call SecurityTokenValidated #207

Closed

OAuthBearerAuthenticationNotifications.SecurityTokenValidated is never triggered #140

Closed

kevinchalet added 2 commits April 13, 2015 23:19

Fix SecurityTokenValidated and rework the different OAuth2 Bearer mid…

4a2a742

…dleware tests

Fix incorrect handler delegation and update SignInScheme documentation

5e03a6c

HaoK merged commit 5e03a6c into aspnet:dev Apr 13, 2015

kevinchalet mentioned this pull request May 14, 2015

Revisit OAuthAuthenticationHandler and add a new SaveTokensAsClaims option #257

Closed

Fix #140, #167, address a comment in #144 and rework the bearer middleware tests #168

Fix #140, #167, address a comment in #144 and rework the bearer middleware tests #168

Uh oh!

Conversation

kevinchalet commented Mar 3, 2015

Uh oh!

ghost commented Mar 3, 2015

Uh oh!

kevinchalet commented Mar 3, 2015

Uh oh!

kevinchalet commented Mar 3, 2015

Uh oh!

Praburaj Mar 5, 2015

Choose a reason for hiding this comment

Uh oh!

HaoK Mar 5, 2015

Choose a reason for hiding this comment

Uh oh!

kevinchalet Mar 5, 2015

Choose a reason for hiding this comment

Uh oh!

Praburaj commented Mar 5, 2015

Uh oh!

brentschmaltz Mar 5, 2015

Choose a reason for hiding this comment

Uh oh!

tushargupta51 commented Mar 5, 2015

Uh oh!

kevinchalet Mar 5, 2015

Choose a reason for hiding this comment

Uh oh!

kevinchalet Mar 5, 2015

Choose a reason for hiding this comment

Uh oh!

kevinchalet commented Mar 10, 2015

Uh oh!

kevinchalet commented Mar 15, 2015

Uh oh!

kevinchalet commented Mar 18, 2015

Uh oh!

HaoK commented Mar 19, 2015

Uh oh!

kevinchalet commented Mar 19, 2015

Uh oh!

kevinchalet commented Apr 9, 2015

Uh oh!

HaoK commented Apr 9, 2015

Uh oh!

HaoK commented Apr 9, 2015

Uh oh!

kevinchalet commented Apr 9, 2015

Uh oh!

HaoK commented Apr 9, 2015

Uh oh!

HaoK commented Apr 9, 2015

Uh oh!

kevinchalet commented Apr 9, 2015

Uh oh!

HaoK commented Apr 9, 2015

Uh oh!

HaoK commented Apr 9, 2015

Uh oh!

Tratcher commented Apr 9, 2015

Uh oh!

brentschmaltz commented Apr 9, 2015

Uh oh!

HaoK commented Apr 9, 2015

Uh oh!

kevinchalet commented Apr 10, 2015

Uh oh!

HaoK commented Apr 13, 2015

Uh oh!

kevinchalet commented Apr 13, 2015

Uh oh!

Uh oh!